-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add focus mode for Navigation Menus #39286
Conversation
Size Change: +1.14 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Exploring... My initial reaction is that this is a very hidden approach to adjusting the Navigation. It should be much more open approach to handling the navigation entities/elements/links. This is not a good approach to use for adjusting the navigation. As it needs to be much more visible when the Navigation block is selected. List view Settings Sidebar. #38600 (comment) I made a video: |
Thanks for taking the time to provide feedback @paaljoachim 👏 🙇♂️
I'd like to challenge that a little. My understanding is that your argument is based on the fact that currently the focus mode is a little hard to discover. Am I right? If so, I agree with that - perhaps the same could be said of all "focus" modes (including Template Part editing). However, just because it's hidden now, does not mean we could not easily expose a more convenient method to access it directly from the block. The key benefits of a focus mode are:
I appreciate this exploration is extremely rough around the edges. However, I'm keen that we don't prematurely dismiss this approach given the value I think it might afford for users. What do you think?
There is a good argument that a list view based approach is a good idea. However, I'm concerned that it's not the ideal environment for direct manupulation and we're going to end up having to duplicate a lot of the functionality we get from blocks within the editor canvas (e.g. styling, text label manipulation, block attribute adjustment...etc). I fear we may bloat the list view if we tried to add all these features. To me we'd be better served to reuse the existing canvas-based block tooling we have available already. That's where the focus mode comes in. I'll also add that I don't see focus mode and sidebar mode as being mutually exclusive (neither does @mtias). They can be complementary. |
Hey Dave I see all focus modes as hidden modes of editing (Except the create a template focus mode). The more direct visible editing we have the better. As it shows it more openly how one would go about editing. Focus mode. Initial view.
(The above is my impression with a few toes in the water so to speak.)
Good point! Improving Focus mode visibility will be helpful. My feedback is based on the experience of focus mode up til this point.
I think that we/I need to look at Focus mode and how we can in general improve this feature. When adding the possibility of using List view to manipulate the Navigation. I could see in the back of my mind that List view would be too limited to all that is needed. Having a separate screen of some kind is still likely needed.
Your various comments swayed me Dave. |
This is cool! But we actually need this for every block, not just navigation. The ellipsis also feels like the right place to add this option, because it can indeed be block agnostic. The main challenge I see there is that the ellipsis menu is already very long, and adding more options to it is tricky. I don't know if this is a blocker, but I'd love input from @jameskoster and @richtabor as well. Some questions to ponder:
Thanks Dave, this is sweet stuff. |
Interesting. Bear in mind this example is designed to sync with the nav item entities. Whereas for most other blocks this would just be standard inner blocks. We could adapt it to make it more flexible though. |
As I noted in #50396 (comment), this experience can be a bit strange given the Navigation styles and settings cannot be included in focus mode. Seeing a horizontal menu on the canvas which jumps to vertical orientation when I click "Edit" is surprising. It kind of suggests I'm editing something else. I know this isn't trivial to fix, but it does concern me a bit about merging this. @jasmussen if we didn't want to clog up the ellipsis menu, we might add an "Edit" item to the toolbar, like template parts: |
I agree we shouldn't be merging this as is. It's still Draft after all 😄 But...it would be good to outline some of the problems/concerns like you have done here @jameskoster. Thank you. |
I do like that edit button. But juxtaposing your note (styles inside the container vs. outside) and my notes from previous conversations that we should be leveraging focus mod wherever possible, I wonder if that Edit pattern works across all containers? |
Question: what is the objective of focus mode? Is it:
If it's the later then we should change the approach from what we've done in this PR. |
The objective of focus mode in general, and speaking here outside of just its navigation use case, is to let you focus on just the contents of this one template part, or group, or pattern, or piece of content. It's the benefit of the reduced nesting, fewer layers to click through, that just makes it overall a more simple editing experience. And this simpler editing experience can benefit all containers, as noted. For the navigation, it may be worth connecting dots to #50396 (comment) which features among other things, this mockup: The user experience is mean to be exactly the same as accessing template parts in the site view. GIF: That is — you just edit the navigtion in isolation. I understand that @jameskoster's comments are appropriate — the navigation might be collapse to a burger inside a template part, but not if edited directly. I still think the flow is valid. Does that answer your questions? |
I sense the tension here comes from the fact that you can see the wrapping (Navigation) block in the UI? Template Parts and Reusable Blocks don't show the wrapping block in focus mode. However the Nav block is unique because we need to show the list view sidebar and basic layout. If we could have "ghost" blocks that are:
...then we could achieve the same type of thing. Using the Nav block does allow us to ensure that the behaviour is consistent though. |
Taking this for a spin, GIF: Lots of thoughts, not all related to this PR but I'll note which ones probably are. The fact that the Back button shows the tooltip when it receives focus is incredibly jarring. I'd love for us to remove the tooltip from those buttons entirely. Visually it is clearly a back button, and for screen readers, the aria-label will read aloud. The ellipsis menu with rename and duplicate are great. It would've been nice if the rename option set focus on the input field, just like the Pages > "Add page" dialog does. Should that same "Add page" dialog be available in navigation menus, with a plus here, next to the ellipsis? I would presume to add a page, and add it to the same menu. Good to fix in this PR: Regardless of plus, we need an Edit icon to edit the particular menu, just like we have an edit button for templates: The ability to click the preview to edit is a bonus, but for anything that's editable there has to always be the edit button too. I realize that 3 icon buttons (Add page, edit, ellipsis) might be a lot, but I believe that @jameskoster has some designs that manages to balance that. This is the main focus mode for editing a navigation: Good to fix in this PR: We should use the navigation icon in the document title, instead of the layout icon, lest you might think you're editing a template part. I think the larges open question that exists is the existential one: the navigation you edit here will not always look the same as it will look on your website, because on your website it might be styled locally in the template part or template. I would love for us to think long term on whether it would be possible to reapply some of those styles in this case (style providers, local, global, etc). But I also still think we should land this:
If we land this and find that it simply doesn't work, we can lean into the Template Parts aspect, where navigation items can be surfaced in the inspector, #51492, and make the Navigation section be more of a "Templates > Manage all templates"-like list view. But it feels too early to make that call. And in the mean time, we can do more to make the discrepancy clear. Good to fix in this PR: for example we can add help text here: Instead of the text:
We could have:
We can even show a subheading and help-text in the focus-mode canvas just like we do for the Style Book (e.g. Headings, etc). |
Fixing unlocking Avoid unnecessarily effectful code Fix bug with persistent Nav block locking Reinstate effect to limit calls to selectBlock Reinstate effect to run selectively Isolate Nav specific code Refactor settings to hook Isolate Editor Canvas to component Extract mode statuses to hook Colocate editor canvas dependencies Remove requirement for Nav hook to return data Extract entire canvas to component Get blocks directly from the store Use factory to provide default editor component Extract independent components for default and wp_navigation Remove template dep from Navigation focus hook Delete redundant hook Remove need for settings prop Extract hooks to files Export boolean to avoid render Remove redundant prop and var Extract Site Editor Canvas component to file Extract factory to file Remove need to pass props to abstract component Improve abstract component and factory naming Improve usage of constants Be explicit about entity mapping in factory Remove templateType prop from SiteEditorCanvas component Improve variable naming Use more performant selector Improve documenting rule for showing appender Move Navigation specific editor settings to relevant provider Remove useSiteEditorMode hook See #39286 (comment) Reintstate bug fix with comment Fix error in rebase Add edit button Use Navigation icon and label Use correct labels
b9dc123
to
04e1104
Compare
@jameskoster thanks for feedback. Here are the actionable tasls I picked out:
It seems like this comment also applies to Template Parts. Why specifically does this become a problem for Navigation? The reason I ask is that I'm keen to avoid too many branches in the code when there might be underlying problems or concepts that we need to reveal. |
Fixes revisions link
I've been looking into why the inserter is empty. Firstly this is due to the If we disable that one for testing however, we still get what I assume would be unexpected behaviour, in that when the Nav block is focused opening the inserter does not insert blocks into the Nav block but rather into the "next" slot on the canvas. I think we need to set the block as the root client id. Ultimately it ends up with a call to |
Template parts also use this region for actions (clearing customisations, deleting), but you're right... the template part inspector is also a bit weak. Probably fine to look into separately.
True. Maybe we can tackle this in #51421.
I noticed the same thing happens when page editing too, and opened #51538. Feel free to edit / expand context there. |
I think it's important to note that the issue this block lacking its surrounding context when editing in focus mode is the same issue that we have in the post editor - the post editor cannot know whether the post will be viewed in a single template or an archive template for example, so users will face the same potential confustion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good. There are a few things we still need to look at, but we should do them in a followup, let's merge and iterate.
* Add new state handlers for setting Nav Menu Fixing unlocking Avoid unnecessarily effectful code Fix bug with persistent Nav block locking Reinstate effect to limit calls to selectBlock Reinstate effect to run selectively Isolate Nav specific code Refactor settings to hook Isolate Editor Canvas to component Extract mode statuses to hook Colocate editor canvas dependencies Remove requirement for Nav hook to return data Extract entire canvas to component Get blocks directly from the store Use factory to provide default editor component Extract independent components for default and wp_navigation Remove template dep from Navigation focus hook Delete redundant hook Remove need for settings prop Extract hooks to files Export boolean to avoid render Remove redundant prop and var Extract Site Editor Canvas component to file Extract factory to file Remove need to pass props to abstract component Improve abstract component and factory naming Improve usage of constants Be explicit about entity mapping in factory Remove templateType prop from SiteEditorCanvas component Improve variable naming Use more performant selector Improve documenting rule for showing appender Move Navigation specific editor settings to relevant provider Remove useSiteEditorMode hook See WordPress#39286 (comment) Reintstate bug fix with comment Fix error in rebase Add edit button Use Navigation icon and label Use correct labels * Add descriptive text as instructed See WordPress#39286 * Update edit link for Navigation post type Fixes revisions link * Fix PHPCS
What
Adds a focus mode to Navigations. Can be accessed via Browse Mode or directly via URL.
Also refactors the Site Editor code to enable this.
Part of #50396.
Why
In #37950 we learnt that a "focus mode" for editing Navigation Menus in isolation could be useful.
This PR implements that experience allowing you to edit a menu's items in the block editor canvas.
Note that because a Navigation Menu can be assigned to multiple blocks, this experience must necessarily be presentationally agonistic (i.e. it's intended to focus more on manipulating the menu items rather than the specific Navigation block instance).
How
Extends the existing infrastructure around Template Parts to also handle Navigation Menu entities. This requires some interesting "hacks" such as using a "ghost" Navigation block to provide the editing context. I anticipate these will be removed in future in favour of a more unified experience but this will be a wider refactor and much more invovled.
Closes #37950.
Testing Instructions
Navigation
and then select one of your menus.Try this with some Themes with Global Styles which effect the styling of the Navigation. You should see the styles inherited in the focus mode.
Todo
wp_navigation
entity.focus
as one of the editor modes. We can then toggle this when entering focus mode and allow us to conditionalise various things about the UI based on whether it is active.Screenshots
Screen.Capture.on.2023-06-12.at.13-53-56.mp4
Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).